src: Improve "bad auth" problems.#150
Conversation
For most status codes of the CYW43_EV_AUTH we assume we have are suffering from a "bad auth". In a congested WiFi environment this can make it look like your WiFi credentials are wrong. Ignore "timeout" and "no ack" status codes as well as "unsolicitated auth packet" as this seems to make a connection more likely to succeed. Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
f144d73 to
95cf7af
Compare
|
This looks OK to me, although I do not fully understand why these events would be generated. The handling of WLC_E_STATUS_SUCCESS and WLC_E_STATUS_UNSOLICITED -- and aborting for all other status values -- follows the WICED driver. But if you have evidence that things are improved with the changes in this PR then I'd be happy to merge it. |
|
I'll keep testing for a bit |
|
I'm seeing this problem downstream and I have a patch that "fixes" some instances of it for MicroPython, but it involves stashing a redundant copy of the SSID/PSK one level down the chain which feels icky. I see this problem pretty much 100% of the time trying to connect to my access point from a Pico 2 W. The first try for every connect will fail with a password failure (surfaced as Anyway assuming this fix does what I think it does... 👍 this (and the usual suite of other transient failures connecting to WiFi networks) has been a thorn in our sides for as long as I can remember. This is pretty grotty: diff --git a/extmod/network_cyw43.c b/extmod/network_cyw43.c
index e1df4fc61..70d9158e1 100644
--- a/extmod/network_cyw43.c
+++ b/extmod/network_cyw43.c
@@ -253,6 +253,54 @@ static mp_obj_t network_cyw43_scan(size_t n_args, const mp_obj_t *pos_args, mp_m
}
static MP_DEFINE_CONST_FUN_OBJ_KW(network_cyw43_scan_obj, 1, network_cyw43_scan);
+// The cyw43 firmware not-uncommonly reports an authentication failure on the
+// very first association to an AP, even when the password is correct: a bad-auth
+// event is quickly followed by a good one, or the WPA handshake times out once.
+// A plain re-join then succeeds. Rather than surface a spurious "wrong password"
+// that every application has to work around with its own retry loop, the join
+// parameters are cached and the join is transparently re-issued a few times
+// before a BADAUTH status is reported to Python. Set to 0 to disable.
+#ifndef MICROPY_PY_NETWORK_CYW43_JOIN_RETRIES
+#define MICROPY_PY_NETWORK_CYW43_JOIN_RETRIES (3)
+#endif
+
+typedef struct _network_cyw43_join_params_t {
+ uint8_t retries_left;
+ bool has_bssid;
+ size_t ssid_len;
+ size_t key_len;
+ uint32_t auth_type;
+ uint32_t channel;
+ uint8_t ssid[32];
+ uint8_t key[64];
+ uint8_t bssid[6];
+} network_cyw43_join_params_t;
+
+static network_cyw43_join_params_t network_cyw43_join_params;
+
+// Re-issue the most recent join using the cached parameters.
+static int network_cyw43_rejoin(network_cyw43_obj_t *self) {
+ network_cyw43_join_params_t *j = &network_cyw43_join_params;
+ return cyw43_wifi_join(self->cyw, j->ssid_len, j->ssid, j->key_len,
+ j->key_len ? j->key : NULL, j->auth_type,
+ j->has_bssid ? j->bssid : NULL, j->channel);
+}
+
+// Return the link status, transparently retrying a transient authentication
+// failure on the STA interface (see network_cyw43_join_params above).
+static int network_cyw43_link_status(network_cyw43_obj_t *self) {
+ int status = cyw43_tcpip_link_status(self->cyw, self->itf);
+ if (self->itf == CYW43_ITF_STA && status == CYW43_LINK_BADAUTH
+ && network_cyw43_join_params.retries_left > 0) {
+ network_cyw43_join_params.retries_left -= 1;
+ if (network_cyw43_rejoin(self) == 0) {
+ // Report the join as still in progress so the caller keeps waiting.
+ return CYW43_LINK_JOIN;
+ }
+ }
+ return status;
+}
+
static mp_obj_t network_cyw43_connect(size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
enum { ARG_ssid, ARG_key, ARG_auth, ARG_security, ARG_bssid, ARG_channel };
static const mp_arg_t allowed_args[] = {
@@ -309,6 +357,28 @@ static mp_obj_t network_cyw43_connect(size_t n_args, const mp_obj_t *pos_args, m
auth_type = args[ARG_security].u_int;
}
+ // Cache the join parameters so a transient authentication failure can be
+ // retried transparently (see network_cyw43_link_status). If the credentials
+ // are too long to cache, auto-retry is simply disabled for this join.
+ network_cyw43_join_params_t *j = &network_cyw43_join_params;
+ if (ssid.len <= sizeof(j->ssid) && key.len <= sizeof(j->key)) {
+ j->ssid_len = ssid.len;
+ memcpy(j->ssid, ssid.buf, ssid.len);
+ j->key_len = key.len;
+ if (key.buf != NULL) {
+ memcpy(j->key, key.buf, key.len);
+ }
+ j->auth_type = auth_type;
+ j->has_bssid = bssid.buf != NULL;
+ if (bssid.buf != NULL) {
+ memcpy(j->bssid, bssid.buf, sizeof(j->bssid));
+ }
+ j->channel = args[ARG_channel].u_int;
+ j->retries_left = MICROPY_PY_NETWORK_CYW43_JOIN_RETRIES;
+ } else {
+ j->retries_left = 0;
+ }
+
// Start the WiFi join procedure. It will run in the background.
int ret = cyw43_wifi_join(self->cyw, ssid.len, ssid.buf, key.len, key.buf,
auth_type, bssid.buf, args[ARG_channel].u_int);
@@ -328,7 +398,7 @@ static MP_DEFINE_CONST_FUN_OBJ_1(network_cyw43_disconnect_obj, network_cyw43_dis
static mp_obj_t network_cyw43_isconnected(mp_obj_t self_in) {
network_cyw43_obj_t *self = MP_OBJ_TO_PTR(self_in);
- bool result = (cyw43_tcpip_link_status(self->cyw, self->itf) == CYW43_LINK_UP);
+ bool result = (network_cyw43_link_status(self) == CYW43_LINK_UP);
if (result && self->itf == CYW43_ITF_AP) {
// For AP we need to not only know if the link is up, but also if any stations
@@ -360,7 +430,7 @@ static mp_obj_t network_cyw43_status(size_t n_args, const mp_obj_t *args) {
if (n_args == 1) {
// no arguments: return link status
- return MP_OBJ_NEW_SMALL_INT(cyw43_tcpip_link_status(self->cyw, self->itf));
+ return MP_OBJ_NEW_SMALL_INT(network_cyw43_link_status(self));
}
// one argument: return status based on query parameter |
|
Regrettably this does not fix my specific issue: The password is not wrong, but if this transient state is hit it seems to latch, never retry and require a new connection attempt. Note, probably related context: #62 (comment) Another note: I arrived here because I'm testing a full refactor of the RM2 module support to meet @dpgeorge's loose design goals: with a bring-up something like |
|
I have absolutely no business digging this deep in the RP2's plumbing, but what do you think of just using bounded retries to soak up transient auth failures? This works 100% of the time every cold boot for me, since it just does - in the cleanest possible way- what I'd do from Python code or as a user prodding the repl: diff --git a/src/cyw43.h b/src/cyw43.h
index 9e3c0ef..98342cd 100644
--- a/src/cyw43.h
+++ b/src/cyw43.h
@@ -122,6 +122,9 @@ typedef struct _cyw43_t {
bool pend_rejoin;
bool pend_rejoin_wpa;
+ // Number of automatic rejoins remaining after a transient join failure.
+ uint8_t wifi_join_retries;
+
// AP settings
uint32_t ap_auth;
uint8_t ap_channel;
diff --git a/src/cyw43_ctrl.c b/src/cyw43_ctrl.c
index 37cc5d5..b0d4a64 100644
--- a/src/cyw43_ctrl.c
+++ b/src/cyw43_ctrl.c
@@ -64,6 +64,15 @@
#define WIFI_JOIN_STATE_KEYED (0x0800)
#define WIFI_JOIN_STATE_ALL (0x0e01)
+// The cyw43 firmware not-uncommonly reports a transient authentication or WPA
+// handshake failure on the first association to an AP, even when the supplied
+// credentials are correct; a plain rejoin then succeeds. Automatically rejoin
+// this many times before reporting a bad-auth failure to the caller. A
+// genuinely wrong password still fails once the retries are exhausted.
+#ifndef CYW43_WIFI_JOIN_AUTH_RETRIES
+#define CYW43_WIFI_JOIN_AUTH_RETRIES (4)
+#endif
+
#define CYW43_STA_IS_ACTIVE(self) (((self)->itf_state >> CYW43_ITF_STA) & 1)
#define CYW43_AP_IS_ACTIVE(self) (((self)->itf_state >> CYW43_ITF_AP) & 1)
@@ -393,8 +402,15 @@ void cyw43_cb_process_async_event(void *cb_data, const cyw43_async_event_t *ev)
// 6 = Unsolicited auth packet
// Ignore it, lets keep trying
} else {
- // Cannot authenticate
- self->wifi_join_state = WIFI_JOIN_STATE_BADAUTH;
+ // Cannot authenticate. This is often transient on a first
+ // association, so rejoin a few times before reporting a bad-auth.
+ if (self->wifi_join_retries > 0) {
+ self->wifi_join_retries -= 1;
+ self->pend_rejoin = true;
+ cyw43_schedule_internal_poll_dispatch(cyw43_poll_func);
+ } else {
+ self->wifi_join_state = WIFI_JOIN_STATE_BADAUTH;
+ }
}
} else if (ev->event_type == CYW43_EV_DEAUTH_IND) {
if (ev->status == 0 && ev->reason == 2) {
@@ -425,8 +441,15 @@ void cyw43_cb_process_async_event(void *cb_data, const cyw43_async_event_t *ev)
self->pend_rejoin = true;
cyw43_schedule_internal_poll_dispatch(cyw43_poll_func);
} else {
- // PSK_SUP failure
- self->wifi_join_state = WIFI_JOIN_STATE_BADAUTH;
+ // PSK_SUP failure. As with EV_AUTH this is often transient, so
+ // rejoin a few times before reporting a bad-auth.
+ if (self->wifi_join_retries > 0) {
+ self->wifi_join_retries -= 1;
+ self->pend_rejoin = true;
+ cyw43_schedule_internal_poll_dispatch(cyw43_poll_func);
+ } else {
+ self->wifi_join_state = WIFI_JOIN_STATE_BADAUTH;
+ }
}
} else if (ev->event_type == CYW43_EV_ICV_ERROR) {
self->pend_rejoin = true;
@@ -654,6 +677,7 @@ int cyw43_wifi_join(cyw43_t *self, size_t ssid_len, const uint8_t *ssid, size_t
// Wait for responses: EV_AUTH, EV_LINK, EV_SET_SSID, EV_PSK_SUP
// Will get EV_DEAUTH_IND if password is invalid
self->wifi_join_state = WIFI_JOIN_STATE_ACTIVE;
+ self->wifi_join_retries = CYW43_WIFI_JOIN_AUTH_RETRIES;
if (auth_type == CYW43_AUTH_OPEN) {
// For open security we don't need EV_PSK_SUP, so set that flag indicator now
|
Were you referring to the change in this PR?
I wouldn't be against the idea. Can you attach / send me a diff and I'll try it out and add it to this PR. |
I was!
I'll push my branch, reduces the chance of me messing up the diff. Edit: Here it is: pimoroni@5e594f8 |
This is a speculative improvement for issues some users seem to see with connecting a Pico to an access point.
Today for some reason the WiFi environment in our office is terrible and I'm seeing a lot of connection problems.
These all show up as "auth failure" as if the WiFi credentials are wrong.
Ignoring "Timeout" and "No ack" status seems to improve reliability.